Skip to content

Access control tests#244

Open
mts1715 wants to merge 14 commits intomainfrom
taras/177-access-control-test
Open

Access control tests#244
mts1715 wants to merge 14 commits intomainfrom
taras/177-access-control-test

Conversation

@mts1715
Copy link
Contributor

@mts1715 mts1715 commented Mar 6, 2026

Closes: #177

Summary

Implements the full Actor Capability Matrix test suite for FlowALPv0.Pool entitlements, as defined in docs/security-permission-matrix.md

Description

cap_test.cdc

The previous file only validated pool creation. This PR replaces it with a structured security test suite covering every row in the permission matrix across all 6 entitlements — one test per matrix operation, organised in sections matching the matrix columns.

Negative test strategy

Cadence entitlements for Pool capabilities (EParticipant, EPosition, ERebalance, EGovernance, EImplementation) are enforced by the cadence type checker.
EPositionAdmin is the only entitlement in this file where negative (access-denied) tests are meaningful at runtime by borrowAuthorizedPosition, so testEPositionAdmin_BorrowUnauthorizedPosition_Fails exists.

Transaction reorganisation

Old pool-management/ and pool-governance/ stubs removed. Replaced by entitlement-scoped folders matching the matrix columns:
egovernance, eimplementation, eparticipant, eposition, epositionadmin, erebalance, setup (grant_e*_cap.cdc helpers), helpers (liquidation)

docs/security-permission-matrix.md

  • Added Test Coverage table mapping each test file to the matrix rows it covers.
  • Added Audit Notes section documenting union/conjunction semantics (EPosition | ERebalance for rebalance ops; FungibleToken.Withdraw + EPositionAdmin conjunction for borrowAuthorizedPosition).
  • Clarified which rows are covered by paid_auto_balance_test.cdc (rebalancer-local entitlements) and withdraw_stability_funds_test.cdc (withdrawStabilityFund).

Known issue documented (not fixed here)

publish_beta_cap.cdc grants EParticipant + EPosition to beta users. EPosition is not needed for normal user actions and allows any beta user to withdraw from or freeze any other user's position. The over-grant is explicitly tested in the eParticipantPositionUser section. Fix: grant EParticipant only — tracked separately.

vishalchangrani and others added 8 commits February 19, 2026 11:56
Maps all FlowALPv0 entitlements to operations by resource, with plain-language descriptions. Intended for audit/security review.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Mark EPosition as protocol-internal only, not for end users
- Add ownership-check warnings on all pool-level EPosition operations
- Document the beta capability over-grant issue (EPosition -> EParticipant fix needed)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace resource-grouped columns with actor columns (User, User w/ EPosition,
Rebalancer, Position Owner, Governance, Protocol Internal). The beta over-grant
is now directly visible as a dedicated column showing what current beta users
can do vs. what they should be able to do.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mts1715 mts1715 self-assigned this Mar 6, 2026
@mts1715 mts1715 requested a review from a team as a code owner March 6, 2026 15:15
Copy link
Member

@Kay-Zee Kay-Zee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few test cases that I would love to have additional verification that the intended action did actually happen, and it was not just that the transaction was successful, but possibly didn't take the intended action, things like balance checks, new value checks, etc. For certain actions where this is harder to do, we can leave for later but the easy ones, that i mentioned i think might make sense to just do here.

Also, would love for the eGovernance tests to have their "negative" case checked, just because those are especially sensitive. It's a little bit "frivolous" as in we're just checking cadence is behaving as expected, but i think it should be pretty easy/straightforward, and gives some added confidence.

import "FlowALPv0"
import "FlowALPModels"

/// TEST TRANSACTION - DO NOT USE IN PRODUCTION
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are all really test transactions only? seems like they technically would still work in prod, no? assuming the account has the right entitlement?

/// Uses the cap stored at FlowALPv0.PoolCapStoragePath.
///
/// NOTE: All logic is in prepare because @Position resources cannot be stored as
/// transaction fields, and execute has no storage access. The prepare-only pattern
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute has no storage access

https://cadence-lang.org/docs/language/transactions#execute-phase

I don't believe that statement is true, si ti? the example on the doc is literally signer.storage.load

Also, why can't @Position resources cannot be stored as transaction fields?


execute {
// Pool.rebalancePosition — requires EPosition | ERebalance; ERebalance alone is sufficient
self.pool.rebalancePosition(pid: pid, force: force)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very similar to the tx above, except goes through the capability, wonder if there's any way to better differentiate the two tx, maybe in naming? anyway, just minor nit.

[setupPid, 1.0],
eParticipantPositionUser
)
Test.expect(result, Test.beSucceeded())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we maybe also do an expect of the balance to make sure the withdraw actually withdrew some amount of tokens, and not just that the withdraw tx succeeded (but didn't withdraw somehow)

[setupPid, 1.0],
eParticipantPositionUser
)
Test.expect(result, Test.beSucceeded())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above, would love to also verify balance


let result = _executeTransaction(
"../tests/transactions/flow-alp/erebalance/rebalance_pool.cdc",
[setupPid, true],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is showcasing that the rebalance user can rebalance the protocol accounts position.

That's what we're trying to show with this test? I thought the rebalance user would only rebalance their own position.

[ePositionAdminPid, TARGET_HEALTH],
ePositionAdminUser
)
Test.expect(result, Test.beSucceeded())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we verify the new target health?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Access Control - Unauthorized access, privilege escalation, entitlement enforcement

3 participants